Skip to content

Add input validation against malformed inputs#293

Open
gtsiolis wants to merge 6 commits into
mainfrom
pro-306-add-input-validation-against-malformed-inputs
Open

Add input validation against malformed inputs#293
gtsiolis wants to merge 6 commits into
mainfrom
pro-306-add-input-validation-against-malformed-inputs

Conversation

@gtsiolis

@gtsiolis gtsiolis commented Jun 9, 2026

Copy link
Copy Markdown
Member

Motivation

lstk is increasingly invoked by AI agents and scripts, which can produce malformed or hostile input (control characters, path traversal, percent-encoding, shell metacharacters) in ways humans rarely do. Pod names, the auth token, and [env.*] config values were only loosely checked (or not at all), so bad input surfaced as a confusing error deep inside a Docker call or platform API request instead of failing fast with a clear reason.

Changes

  • Add internal/validate, reusable deterministic validators (ResourceName, EnvVarName, NoControlChars, AuthToken) that return a machine-classifiable *Error (stable Rule field) instead of a generic message.
  • Wire validate.AuthToken into cmd/root.go's token resolution (env/keyring): trims whitespace, then rejects control chars, embedded whitespace, or implausibly long tokens.
  • Wire validate.EnvVarName/NoControlChars into internal/config/config.go to reject malformed [env.*] keys/values before they're injected into a container's environment.
  • Replace internal/snapshot's bare pod-name regex with validate.ResourceName, adding deny-checks for percent-encoding, path traversal, embedded path/query characters, and shell metacharacters.
  • Rebase conflict resolution: current main had independently grown its own pod-name validator (ValidatePodName, from the S3 remote-storage PR Add S3 remote storage for snapshot save, load, and list #344) that permits underscores and is also called directly for S3-remote pod names in cmd/snapshot.go. ValidatePodName now delegates to validate.ResourceName, whose charset was widened to keep underscores working — the S3-remote pod-name path now gets the same deny-checks as pod: refs, which it didn't before.
  • Document the new package in the root CLAUDE.md, and encode its usage as a convention in the agent instructions so future input surfaces route through it instead of growing parallel validators: a Code Style rule in CLAUDE.md (route by value class — identifiers → ResourceName; opaque secrets → loose AuthToken-style checks; paths/URLs → their existing parsers; extend internal/validate rather than inlining a regexp), an input question + wiring/test steps in the add-command skill, and a checklist item in the review-pr skill.

Review

Human review advised — changes user-facing error behavior for auth tokens, pod names, and [env.*] config (previously-accepted values are now rejected with new/different messages), and this rebase merges two independently-evolved pod-name validators.

Tests

New unit tests in internal/validate/validate_test.go (all validators, deny-check ordering, rule codes) and internal/config/env_validation_test.go; extended internal/snapshot/destination_test.go with malformed pod-name cases (percent-encoding, embedded query, shell metacharacters) plus a regression case confirming underscore-named pods still pass. Verified after rebase: make build, go vet ./..., make test (full suite), make lint (both modules) all green.

Closes PRO-306

@gtsiolis gtsiolis self-assigned this Jun 9, 2026
@gtsiolis
gtsiolis force-pushed the pro-306-add-input-validation-against-malformed-inputs branch 2 times, most recently from 76c5d3e to 18243ab Compare July 1, 2026 07:04
@gtsiolis gtsiolis added the docs: skip Pull request does not require documentation changes label Jul 1, 2026
@gtsiolis
gtsiolis force-pushed the pro-306-add-input-validation-against-malformed-inputs branch 4 times, most recently from 9dc3932 to 23fb7d2 Compare July 8, 2026 07:10
@gtsiolis
gtsiolis force-pushed the pro-306-add-input-validation-against-malformed-inputs branch 2 times, most recently from fd157f8 to 066c308 Compare July 13, 2026 07:08
@gtsiolis
gtsiolis force-pushed the pro-306-add-input-validation-against-malformed-inputs branch 5 times, most recently from b274940 to f4cd609 Compare July 22, 2026 13:03
@gtsiolis
gtsiolis marked this pull request as ready for review July 22, 2026 13:10
@gtsiolis
gtsiolis requested a review from a team as a code owner July 22, 2026 13:10
@gtsiolis

Copy link
Copy Markdown
Member Author

Updated PR description. I'd appreciate a human review following the suggestion from the PR description. 🤖

@gtsiolis
gtsiolis force-pushed the pro-306-add-input-validation-against-malformed-inputs branch from 9c0398f to f2a04f9 Compare July 22, 2026 14:02
@gtsiolis

Copy link
Copy Markdown
Member Author

:fyi: the last commit (f2a04f9) is instructions only, no runtime change.

It turns the validation approach into a documented convention: a Code Style rule in CLAUDE.md routing values by class (identifiers through validate.ResourceName, opaque secrets through loose AuthToken-style checks, paths and URLs through their existing parsers), plus matching scaffold and test steps in the add-command skill and a checklist item in the review-pr skill.

Why: the pod name rules had already forked once while this branch was open (#344 grew its own validator), so this points new input surfaces at internal/validate instead of letting parallel regexps drift again.

@gtsiolis
gtsiolis force-pushed the pro-306-add-input-validation-against-malformed-inputs branch from f2a04f9 to 8434e08 Compare July 23, 2026 07:10

@skyrpex skyrpex left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The only thing that triggers my spider-sense is maybe building over-specific validation for generic inputs, for example:

Are we certain that a ResourceName will never contain .., /, ? or #?

gtsiolis and others added 2 commits July 23, 2026 16:12
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@gtsiolis

Copy link
Copy Markdown
Member Author

Good spider-sense! 🕸️

Did some investigation on this, and confirmed that AWS has no universal resource-name grammar. S3 object keys can contain /, ?, and valid .. segments, and CloudWatch log group names can even contain #, so a generic validator can't be right for those.

The validator in this PR only guards Cloud Pod names, and naming it ResourceName could have caused confusion. I renamed it to PodName and updated the conventions to route other identifiers to their owning API's contract.

For pod names we can be certain since the platform enforces ^[a-zA-Z0-9_-]+$ (see verify_pod_name in localstack-platform, same pattern in the emulator's pods bootstrap), and the name is embedded in the URL path (/v1/cloudpods/<name>), so .., /, ?, # can't appear in a valid name.

I aligned the allow-list to that exact pattern, which also fixes a real over-restriction: we previously rejected leading _/- names the platform accepts.

I'd appreciate a second round when you have some time, @skyrpex! 🙏

@gtsiolis
gtsiolis requested a review from skyrpex July 23, 2026 13:43
Co-Authored-By: Claude <noreply@anthropic.com>
@gtsiolis

Copy link
Copy Markdown
Member Author

Pushed one more commit to fix the pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs: skip Pull request does not require documentation changes semver: patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants